Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Added Cmm_helpers.Scalar_type #3423

Merged
merged 2 commits into from
Mar 26, 2025
Merged

Added Cmm_helpers.Scalar_type #3423

merged 2 commits into from
Mar 26, 2025

Conversation

jvanburen
Copy link
Contributor

Cmm_helpers.Scalar_type provides utilities for converting between integers types of different widths and signedness. This is in preparation for adding unboxed small integer types.

@jvanburen jvanburen requested review from mshinwell and Gbury and removed request for mshinwell January 3, 2025 16:04
@mshinwell mshinwell added the cmm Cmm language / helpers changes label Jan 15, 2025
@github-actions GitHub Actions
Copy link

Selection Change Check

This PR modifies the original selection pass (targeting Mach).
Please check whether the changes should also be applied to the
CFG variant of the pass.

@jvanburen jvanburen force-pushed the cmm-refactor-unboxed-fields branch from 16f1b23 to 443a81c Compare January 29, 2025 17:14
@jvanburen jvanburen force-pushed the cmm-refactor-unboxed-fields branch from 443a81c to 2f5d60b Compare February 13, 2025 22:45
Base automatically changed from cmm-refactor-unboxed-fields to main February 18, 2025 16:42
@jvanburen jvanburen force-pushed the cmm-scalar-type branch 2 times, most recently from 1d28ccf to e856bc9 Compare February 18, 2025 17:47
@jvanburen jvanburen requested a review from TheNumbat February 21, 2025 20:53
@TheNumbat TheNumbat changed the base branch from main to refactor-cmm-for-more-integer-widths February 25, 2025 19:02
TheNumbat
TheNumbat reviewed Feb 25, 2025
View reviewed changes
Copy link
Contributor

@TheNumbat TheNumbat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, I'm confident the conversions are correct. Have you checked that this doesn't hurt codegen for static casts? We don't have a good way to test that automatically, but spot-checking a few things seems sufficient.

@jvanburen
Copy link
Contributor Author

Looks good, I'm confident the conversions are correct. Have you checked that this doesn't hurt codegen for static casts? We don't have a good way to test that automatically, but spot-checking a few things seems sufficient.

I noticed that the generated Cors could mess up the generation of lea for tagging, so I fixed that case in instruction selection.
Otherwise I didn't see anything meaningful.

@mshinwell
Copy link
Collaborator

Should we e.g. examine the actual generated assembly for a few executables before and after this change? I think probably yes.

Base automatically changed from refactor-cmm-for-more-integer-widths to main March 5, 2025 16:57
@jvanburen jvanburen marked this pull request as draft March 5, 2025 20:57
Gbury
Gbury requested changes Mar 6, 2025
View reviewed changes
@Gbury
Copy link
Contributor

Gbury commented Mar 6, 2025

Also: there's a CI failure that seems related to this PR: Fatal error: exception Stdlib__Domain.Encapsulated("Invalid_argument(\"Random.int32_in_range\")") on chi2.ml in the upstream testsuite

Gbury
Gbury reviewed Mar 7, 2025
View reviewed changes
Copy link
Contributor

@Gbury Gbury left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I wait until you fix the CI failures to review again ?

@jvanburen jvanburen force-pushed the cmm-scalar-type branch 5 times, most recently from 8fa9539 to 6dce5c0 Compare March 10, 2025 21:29
@jvanburen jvanburen force-pushed the cmm-scalar-type branch 4 times, most recently from 5f3896a to 69edaea Compare March 24, 2025 18:39
@jvanburen
Added Cmm_helpers.Scalar_type. This provides utilities for converti…
176df12 
…ng between integers types of different widths and signedness. This is in preparation for adding unboxed small integer types.
@jvanburen jvanburen marked this pull request as ready for review March 26, 2025 13:49
Gbury
Gbury requested changes Mar 26, 2025
View reviewed changes
Copy link
Contributor

@Gbury Gbury left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to go apart from one stylistic comment.

@jvanburen jvanburen enabled auto-merge (squash) March 26, 2025 18:02
@jvanburen jvanburen merged commit 365fe33 into main Mar 26, 2025
22 checks passed
@jvanburen jvanburen deleted the cmm-scalar-type branch March 26, 2025 18:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mshinwell mshinwell mshinwell left review comments

@Gbury Gbury Gbury approved these changes

@TheNumbat TheNumbat TheNumbat approved these changes

Assignees
No one assigned
Labels
cmm Cmm language / helpers changes needs testing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jvanburen @mshinwell @Gbury @TheNumbat